SEC-07: SSO/OIDC integration with optional MFA policy#813
Conversation
Domain layer for TOTP-based MFA: MfaCredential entity tracks per-user TOTP secrets with confirmation state and recovery codes. User entity gains MfaEnabled boolean and EnableMfa/DisableMfa methods.
OidcProviderSettings for pluggable OIDC provider configs, MfaPolicySettings for optional MFA policy, MfaService with TOTP generation/validation, IMfaCredentialRepository, and supporting DTOs.
MfaCredentialRepository, EF configuration, migration adding MfaCredentials table and User.MfaEnabled column, UnitOfWork and DI registration.
MfaController with setup/confirm/verify/disable endpoints, OIDC login/callback endpoints in AuthController, OpenIdConnect provider registration, settings binding for OIDC and MFA policy configuration.
…changes Add MfaCredentials property to FakeUnitOfWork implementations across test files. Update AuthController instantiation to include OidcSettings parameter.
MfaServiceTests covering setup, confirm, disable, verify, TOTP validation, base32 encoding, status, and policy checks. OidcSecurityTests covering provider validation, email collision protection, cross-provider isolation, username deduplication, inactive user rejection, and config validation. MfaCredentialTests for domain entity validation. User MFA flag tests.
OIDC login buttons on LoginView (config-gated), MFA types and API client methods, MfaSetup component for settings, MfaChallengeModal for protected action verification, sessionStore OIDC code exchange.
Documents design decisions for pluggable OIDC provider support, TOTP-based MFA, identity mapping strategy, and authorization code flow.
EF Core requires a designer file alongside each migration for the model snapshot at that migration point.
Adversarial Security ReviewCRITICAL FindingsNone identified. HIGH Findings1. TOTP secret stored in plaintext in database (HIGH)
2. No TOTP replay protection (HIGH)
MEDIUM Findings3. Unused
4. MFA setup race condition (MEDIUM)
5. Recovery code " " (space) sentinel value (MEDIUM)
LOW Findings6. Provider name in error messages could leak configuration (LOW)
7.
Verification
|
There was a problem hiding this comment.
Pull request overview
This PR introduces pluggable OIDC SSO sign-in (multi-provider, config-gated) and optional TOTP-based MFA (setup + verification + disable), along with supporting persistence, API endpoints, UI components, and ADR documentation.
Changes:
- Add OIDC provider configuration/registration and new auth endpoints (providers listing, OIDC login/callback/exchange).
- Add MFA domain model + persistence (EF config + migration) and application/service + API endpoints for status/setup/confirm/verify/disable.
- Add frontend UI for OIDC sign-in buttons and MFA setup/challenge, plus extensive backend test coverage and ADR-0028.
Reviewed changes
Copilot reviewed 45 out of 46 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
| frontend/taskdeck-web/src/views/LoginView.vue | Adds config-gated OIDC provider buttons and wiring for provider discovery. |
| frontend/taskdeck-web/src/types/auth.ts | Adds OIDC provider, MFA status/setup/verify types. |
| frontend/taskdeck-web/src/store/sessionStore.ts | Adds OIDC code exchange action to session store. |
| frontend/taskdeck-web/src/components/MfaSetup.vue | New MFA setup/confirm/disable UI component. |
| frontend/taskdeck-web/src/components/MfaChallengeModal.vue | New modal UI to verify MFA for sensitive actions. |
| frontend/taskdeck-web/src/api/authApi.ts | Adds OIDC exchange + MFA API client methods. |
| docs/decisions/INDEX.md | Adds ADR-0028 to decisions index. |
| docs/decisions/ADR-0028-sso-oidc-mfa-integration.md | Documents design decisions for OIDC + optional MFA integration. |
| backend/tests/Taskdeck.Domain.Tests/Entities/UserTests.cs | Tests default and toggling behavior for User.MfaEnabled. |
| backend/tests/Taskdeck.Domain.Tests/Entities/MfaCredentialTests.cs | Adds unit tests for the new MfaCredential entity. |
| backend/tests/Taskdeck.Application.Tests/Services/OidcSecurityTests.cs | Adds security tests for external login/OIDC claim mapping and collision protections. |
| backend/tests/Taskdeck.Application.Tests/Services/MfaServiceTests.cs | Adds tests for MFA setup/confirm/verify/disable and helpers (Base32/TOTP). |
| backend/tests/Taskdeck.Api.Tests/WorkerResilienceTests.cs | Updates fake unit-of-work to include MFA repository property. |
| backend/tests/Taskdeck.Api.Tests/ProposalHousekeepingWorkerTests.cs | Updates fake unit-of-work to include MFA repository property. |
| backend/tests/Taskdeck.Api.Tests/ProposalHousekeepingWorkerEdgeCaseTests.cs | Updates fake unit-of-work to include MFA repository property. |
| backend/tests/Taskdeck.Api.Tests/OutboundWebhookHmacDeliveryTests.cs | Updates stub unit-of-work to include MFA repository property. |
| backend/tests/Taskdeck.Api.Tests/OutboundWebhookDeliveryWorkerTests.cs | Updates fake unit-of-work to include MFA repository property. |
| backend/tests/Taskdeck.Api.Tests/OutboundWebhookDeliveryWorkerReliabilityTests.cs | Updates fake unit-of-work to include MFA repository property. |
| backend/tests/Taskdeck.Api.Tests/LlmQueueToProposalWorkerTests.cs | Updates fake unit-of-work to include MFA repository property. |
| backend/tests/Taskdeck.Api.Tests/AuthControllerEdgeCaseTests.cs | Updates controller construction to include OidcSettings. |
| backend/tests/Taskdeck.Api.Tests/ActiveUserValidationMiddlewareTests.cs | Updates stub unit-of-work to include MFA repository property. |
| backend/src/Taskdeck.Infrastructure/Repositories/UnitOfWork.cs | Adds MfaCredentials repository to unit-of-work implementation. |
| backend/src/Taskdeck.Infrastructure/Repositories/MfaCredentialRepository.cs | Implements repository for MFA credentials. |
| backend/src/Taskdeck.Infrastructure/Persistence/TaskdeckDbContext.cs | Adds DbSet<MfaCredential>. |
| backend/src/Taskdeck.Infrastructure/Persistence/Configurations/UserConfiguration.cs | Persists User.MfaEnabled with default false. |
| backend/src/Taskdeck.Infrastructure/Persistence/Configurations/MfaCredentialConfiguration.cs | EF Core mapping for MFA credentials, unique per-user. |
| backend/src/Taskdeck.Infrastructure/Migrations/TaskdeckDbContextModelSnapshot.cs | Updates snapshot for MFA credential table and user flag. |
| backend/src/Taskdeck.Infrastructure/Migrations/20260409120000_AddMfaCredentials.Designer.cs | Auto-generated migration designer for MFA table + user column. |
| backend/src/Taskdeck.Infrastructure/Migrations/20260409120000_AddMfaCredentials.cs | Adds MfaCredentials table and Users.MfaEnabled column. |
| backend/src/Taskdeck.Infrastructure/DependencyInjection.cs | Registers IMfaCredentialRepository. |
| backend/src/Taskdeck.Domain/Entities/User.cs | Adds MfaEnabled + enable/disable methods. |
| backend/src/Taskdeck.Domain/Entities/MfaCredential.cs | New MFA credential entity (secret + confirmation + recovery codes). |
| backend/src/Taskdeck.Application/Services/OidcProviderSettings.cs | Adds OIDC provider config + configured-provider filtering. |
| backend/src/Taskdeck.Application/Services/MfaService.cs | Implements MFA setup/confirm/verify/disable, TOTP + recovery code logic. |
| backend/src/Taskdeck.Application/Services/MfaPolicySettings.cs | Adds MFA policy settings (setup enablement + sensitive action requirements). |
| backend/src/Taskdeck.Application/Interfaces/IUnitOfWork.cs | Adds MfaCredentials repository property. |
| backend/src/Taskdeck.Application/Interfaces/IMfaCredentialRepository.cs | New repository interface for MFA credentials. |
| backend/src/Taskdeck.Application/DTOs/OidcDtos.cs | DTO for exposing configured OIDC provider info to frontend. |
| backend/src/Taskdeck.Application/DTOs/MfaDtos.cs | DTOs for MFA setup/status/verify requests. |
| backend/src/Taskdeck.Api/Taskdeck.Api.csproj | Adds OpenIdConnect authentication package reference. |
| backend/src/Taskdeck.Api/Program.cs | Wires OIDC settings into authentication registration. |
| backend/src/Taskdeck.Api/Extensions/SettingsRegistration.cs | Binds/registers OIDC + MFA policy settings. |
| backend/src/Taskdeck.Api/Extensions/AuthenticationRegistration.cs | Registers OpenIdConnect providers (config-gated). |
| backend/src/Taskdeck.Api/Extensions/ApplicationServiceRegistration.cs | Registers MfaService. |
| backend/src/Taskdeck.Api/Controllers/MfaController.cs | Adds authenticated MFA endpoints (status/setup/confirm/verify/disable). |
| backend/src/Taskdeck.Api/Controllers/AuthController.cs | Adds providers endpoint output + OIDC login/callback/exchange endpoints. |
Files not reviewed (1)
- backend/src/Taskdeck.Infrastructure/Migrations/20260409120000_AddMfaCredentials.Designer.cs: Language not supported
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| hashedCodes.RemoveAt(i); | ||
| credential.SetRecoveryCodes(hashedCodes.Count > 0 | ||
| ? string.Join(",", hashedCodes) | ||
| : " "); // Keep non-empty to satisfy domain validation | ||
| return true; |
There was a problem hiding this comment.
When the last recovery code is used, this calls credential.SetRecoveryCodes(" "). SetRecoveryCodes rejects whitespace-only values (string.IsNullOrWhiteSpace), so this will throw and turn a successful recovery-code verification into a server error. Store null/empty to represent “no codes left” (and update the domain method accordingly), or add a dedicated method to clear recovery codes safely.
| /// <summary> | ||
| /// Stores a TOTP-based MFA credential for a user. | ||
| /// Each user may have at most one active TOTP credential. | ||
| /// The shared secret is stored encrypted at rest by the infrastructure layer. | ||
| /// </summary> |
There was a problem hiding this comment.
MfaCredential’s doc comment claims the shared secret is “stored encrypted at rest by the infrastructure layer”, but in this PR the EF configuration persists Secret as a plain string with no value converter/encryption. Either implement encryption at rest (e.g., EF Core value converter with data protection) or update the documentation to avoid giving a false security guarantee.
| public record ChangePasswordRequest(string CurrentPassword, string NewPassword); | ||
| public record ExchangeCodeRequest(string Code); | ||
| public record OidcExchangeCodeRequest(string Code, string Provider); | ||
|
|
There was a problem hiding this comment.
OidcExchangeCodeRequest is declared but never used (the exchange endpoint accepts ExchangeCodeRequest). This adds dead code and can confuse consumers about the required request shape. Remove it, or switch the endpoint to use it if you intend to require the provider name on exchange.
| const route = useRoute() | ||
| const session = useSessionStore() | ||
|
|
||
| import type { OidcProviderInfo } from '../types/auth' |
There was a problem hiding this comment.
import type { OidcProviderInfo } ... appears after executable statements (const router = ...). In <script setup>, imports must come before other statements; as-is this will fail to compile/typecheck. Move the import up with the other imports.
| <p class="td-mfa-setup__step"> | ||
| 1. Scan the QR code below with your authenticator app (Google Authenticator, Authy, etc.) | ||
| </p> | ||
| <div class="td-mfa-setup__qr"> | ||
| <code class="td-mfa-setup__secret">{{ setupResponse.sharedSecret }}</code> |
There was a problem hiding this comment.
The setup wizard instructs users to “Scan the QR code below”, but no QR code is rendered and setupResponse.qrCodeUri is never used—only the secret is displayed. Either render a QR code from qrCodeUri or adjust the copy so it matches the UI.
| authBuilder.AddOpenIdConnect(schemeName, provider.DisplayName, options => | ||
| { | ||
| options.Authority = provider.Authority; | ||
| options.ClientId = provider.ClientId; | ||
| options.ClientSecret = provider.ClientSecret; |
There was a problem hiding this comment.
The new OIDC handler relies on a sign-in scheme to persist the external principal between the middleware callback path and the controller callback. This AddOpenIdConnect setup does not set options.SignInScheme, and the app doesn’t register any cookie/default sign-in scheme, so OIDC (and GitHub) remote auth will fail at runtime. Add a dedicated cookie scheme (e.g., "External") and set it as DefaultSignInScheme / options.SignInScheme.
| /// <summary> | ||
| /// Revokes this MFA credential by clearing the secret and marking as unconfirmed. | ||
| /// The entity remains for audit trail purposes. | ||
| /// </summary> | ||
| public void Revoke() |
There was a problem hiding this comment.
Revoke() is documented as “clearing the secret”, but the implementation only flips IsConfirmed and leaves Secret / recovery codes intact. Either clear sensitive fields when revoking (and consider making it irreversible), or update the doc comment to match the behavior.
| function startOidcLogin(providerName: string) { | ||
| const apiBase = import.meta.env.VITE_API_BASE_URL || 'http://localhost:5000/api' | ||
| const redirect = [route.query.redirect].flat()[0] | ||
| const returnUrl = redirect | ||
| ? `/login?redirect=${encodeURIComponent(redirect)}` |
There was a problem hiding this comment.
This starts the OIDC login flow, but the page currently treats any returned oauth_code as a GitHub sign-in (exchange endpoint + user-facing messages). That will make OIDC sign-in UX misleading and leaves the OIDC exchange path unused. Consider adding a provider discriminator in the redirect (or switching exchange based on source) and updating status/error text accordingly.
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces support for OIDC-based SSO and TOTP-based Multi-Factor Authentication (MFA). It adds the necessary infrastructure, domain entities, and API endpoints to support these features, including configuration-gated OIDC providers and MFA policy settings. The review identified several areas for improvement, including increasing the entropy of generated recovery codes, removing unused code, refactoring duplicated logic, and addressing inconsistencies in domain validation and entity management.
| // Generate an 8-character alphanumeric recovery code in two groups: XXXX-XXXX | ||
| var bytes = RandomNumberGenerator.GetBytes(5); | ||
| var hex = Convert.ToHexString(bytes).ToUpperInvariant()[..8]; | ||
| codes[i] = $"{hex[..4]}-{hex[4..8]}"; |
There was a problem hiding this comment.
The generated recovery codes have relatively low entropy. An 8-character hex string provides 32 bits of entropy (16^8 = 2^32 possibilities). While these are single-use and the hashes are stored, an attacker who can bypass rate limiting could potentially brute-force a code. Consider increasing the entropy by generating longer codes. For example, using 8 bytes from RandomNumberGenerator would produce a 16-character hex string with 64 bits of entropy, which is significantly more secure.
// Generate a 16-character alphanumeric recovery code in two groups: XXXXXXXX-XXXXXXXX
var bytes = RandomNumberGenerator.GetBytes(8);
var hex = Convert.ToHexString(bytes).ToUpperInvariant();
codes[i] = $"{hex[..8]}-{hex[8..]}";|
|
||
| public record ChangePasswordRequest(string CurrentPassword, string NewPassword); | ||
| public record ExchangeCodeRequest(string Code); | ||
| public record OidcExchangeCodeRequest(string Code, string Provider); |
There was a problem hiding this comment.
This record OidcExchangeCodeRequest appears to be unused. The OidcExchangeCode action method uses ExchangeCodeRequest instead. To avoid confusion and dead code, this record should be removed.
| public IActionResult OidcExchangeCode([FromBody] ExchangeCodeRequest request) | ||
| { | ||
| if (string.IsNullOrWhiteSpace(request.Code)) | ||
| return BadRequest(new ApiErrorResponse(ErrorCodes.ValidationError, "Code is required")); | ||
|
|
||
| if (!_authCodes.TryRemove(request.Code, out var entry)) | ||
| return Unauthorized(new ApiErrorResponse(ErrorCodes.AuthenticationFailed, "Invalid or expired code")); | ||
|
|
||
| if (DateTimeOffset.UtcNow > entry.Expiry) | ||
| return Unauthorized(new ApiErrorResponse(ErrorCodes.AuthenticationFailed, "Code has expired")); | ||
|
|
||
| return Ok(entry.Result); | ||
| } |
There was a problem hiding this comment.
This method OidcExchangeCode is identical to the existing ExchangeCode method for GitHub. This code duplication can be avoided by extracting the logic into a private helper method that both actions can call. This would improve maintainability and reduce the chance of bugs if one method is updated but the other is not.
For example, you could create a private method:
private IActionResult ExchangeCodeInternal(ExchangeCodeRequest request)
{
if (string.IsNullOrWhiteSpace(request.Code))
return BadRequest(new ApiErrorResponse(ErrorCodes.ValidationError, "Code is required"));
if (!_authCodes.TryRemove(request.Code, out var entry))
return Unauthorized(new ApiErrorResponse(ErrorCodes.AuthenticationFailed, "Invalid or expired code"));
if (DateTimeOffset.UtcNow > entry.Expiry)
return Unauthorized(new ApiErrorResponse(ErrorCodes.AuthenticationFailed, "Code has expired"));
return Ok(entry.Result);
}Then both ExchangeCode and OidcExchangeCode can be simplified to one-line calls to this helper.
| var callbackPath = !string.IsNullOrWhiteSpace(provider.CallbackPath) | ||
| ? provider.CallbackPath | ||
| : $"/api/auth/oidc/{provider.Name.ToLowerInvariant()}/oauth-redirect"; |
There was a problem hiding this comment.
The logic to construct a default CallbackPath is a bit confusing. The default path /api/auth/oidc/{providerName}/oauth-redirect is not an endpoint handled by a controller and might be difficult for users to discover when configuring their IdP.
Consider removing this custom default and letting the OpenID Connect middleware use its standard default path, which is typically /signin-oidc. This simplifies configuration for the user, as they would only need to register one callback URL for all OIDC providers unless they explicitly override it in the configuration.
| hashedCodes.RemoveAt(i); | ||
| credential.SetRecoveryCodes(hashedCodes.Count > 0 | ||
| ? string.Join(",", hashedCodes) | ||
| : " "); // Keep non-empty to satisfy domain validation |
There was a problem hiding this comment.
Using a single space " " to bypass the domain validation for an empty list of recovery codes is a workaround that can be confusing. It would be cleaner to adjust the domain validation in MfaCredential.SetRecoveryCodes to allow an empty string, signifying that no recovery codes are left. This would make the logic here more straightforward and remove the need for this magic value.
: string.Empty); // Keep non-empty to satisfy domain validation| internal static string Base32Encode(byte[] data) | ||
| { | ||
| var result = new char[(data.Length * 8 + 4) / 5]; | ||
| var buffer = 0; | ||
| var bitsLeft = 0; | ||
| var index = 0; | ||
|
|
||
| foreach (var b in data) | ||
| { | ||
| buffer = (buffer << 8) | b; | ||
| bitsLeft += 8; | ||
|
|
||
| while (bitsLeft >= 5) | ||
| { | ||
| bitsLeft -= 5; | ||
| result[index++] = Base32Chars[(buffer >> bitsLeft) & 0x1F]; | ||
| } | ||
| } | ||
|
|
||
| if (bitsLeft > 0) | ||
| { | ||
| buffer <<= (5 - bitsLeft); | ||
| result[index] = Base32Chars[buffer & 0x1F]; | ||
| } | ||
|
|
||
| return new string(result); | ||
| } |
There was a problem hiding this comment.
Implementing custom encoding/decoding logic like Base32 can be risky. While this implementation appears reasonable, it's generally recommended to use a well-vetted, standard library for such operations to avoid potential subtle bugs and security issues. Consider replacing this custom implementation with a popular NuGet package like Base32 to improve robustness and security.
| public void Revoke() | ||
| { | ||
| IsConfirmed = false; | ||
| Touch(); | ||
| } |
There was a problem hiding this comment.
This Revoke() method appears to be unused. The MfaService.DisableAsync method deletes the MfaCredential entity directly instead of revoking it. Furthermore, the comment here states 'The entity remains for audit trail purposes', which contradicts the deletion behavior in the service.
Please either use this Revoke() method in the service to align with the audit trail goal, or remove this method and its comment to eliminate dead code and confusion.
Add TOTP replay protection using ConcurrentDictionary to track used codes within the validity window. Remove unused OidcExchangeCodeRequest record. Replace space sentinel with EXHAUSTED constant for exhausted recovery codes.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ea9c57ba19
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| credential.SetRecoveryCodes(hashedCodes.Count > 0 | ||
| ? string.Join(",", hashedCodes) | ||
| : " "); // Keep non-empty to satisfy domain validation |
There was a problem hiding this comment.
When the final recovery code is used, hashedCodes.Count becomes 0 and this branch writes a single space to SetRecoveryCodes. MfaCredential.SetRecoveryCodes rejects whitespace-only values, so this path throws a DomainException instead of returning a normal verification result. In practice, the user's last valid recovery code triggers a 500 from VerifyCodeAsync/MfaController.Verify rather than succeeding cleanly.
Useful? React with 👍 / 👎.
| var result = await _mfaService.VerifyCodeAsync(userId, request.Code); | ||
| return result.IsSuccess ? NoContent() : result.ToErrorActionResult(); |
There was a problem hiding this comment.
This endpoint only returns 204 NoContent after code validation and does not mint/store any challenge proof tied to the session or action, so sensitive operations cannot enforce that MFA was just satisfied. As implemented, a client can still call sensitive APIs directly with only the JWT, meaning RequireMfaForSensitiveActions has no effective server-side enforcement path.
Useful? React with 👍 / 👎.
Security Review Fixes AppliedThe following findings from the adversarial review have been addressed in commit e3ba9a1: HIGH: TOTP replay protection -- Fixed. Added MEDIUM: Unused MEDIUM: Recovery code sentinel value -- Fixed. Replaced All 3,805 backend tests continue to pass. Frontend typecheck and 1,898 tests also pass. |
Independent Adversarial Review (Round 2) -- Security-FocusedCRITICALC1. TOTP shared secret stored in plaintext in databaseFile: The Fix: Add an EF Core value converter using C2. MFA policy for sensitive actions is dead code -- no enforcement existsFile:
The PR description and ADR claim MFA gates sensitive actions. This is currently false -- the entire MFA enforcement pipeline is unconnected. An attacker with a stolen JWT can change passwords and delete accounts without MFA challenge. Fix: Wire HIGHH1. DisableAsync does not accept recovery codes -- users can be permanently locked outFile:
This creates a permanent lockout scenario: user enables MFA, loses phone, has recovery codes, but cannot disable MFA to regain account access. The only recovery path is direct database intervention. Fix: After H2. Frontend OIDC code exchange uses wrong endpointFile: When the OIDC callback redirects back with This works accidentally because both endpoints share the same
Fix: Detect whether the MEDIUMM1. No per-user failed MFA attempt counter or account lockoutFile: The MFA verify endpoint relies solely on the There is no per-user failed attempt counter, no exponential backoff on failures, and no temporary account lockout after N failures. NIST SP 800-63B recommends rate limiting MFA verification to prevent brute-force attacks on the second factor. Fix: Add a per-user failed attempt counter. After 5-10 consecutive failures, impose a progressive delay or temporary lockout (e.g., 15 minutes). Reset on successful verification. M2. Recovery code entropy is low (32 bits)File: Recovery codes are generated from 5 random bytes, hex-encoded, and truncated to 8 characters (format: XXXX-XXXX). This gives 32 bits of entropy per code. Industry standard (GitHub, Google, Microsoft) uses 10+ character alphanumeric codes with ~48-64 bits of entropy. With 8 recovery codes at 32 bits each, and BCrypt verification being slow (preventing brute force on individual codes), this is mitigated but still below common standards. Fix: Use M3. No provider name sanitization in OIDC configurationFile: The Fix: Add a regex validation in LOWL1. Static _usedCodes dictionary grows unboundedly under loadFile: The Fix: Add a periodic cleanup or size cap, or cleanup on both successful and failed attempts. L2. MfaSetup.vue exposes raw TOTP secret without QR code renderingFile: The setup component displays the raw Base32 secret in a L3. _authCodes not partitioned between GitHub and OIDCFile: The same INFOI1. OIDC token validation relies on ASP.NET Core defaults (adequate)The I2. Open redirect protection is correctly double-validatedThe I3. Email collision prevention is correctly implementedThe I4. Recovery codes are correctly hashed with BCrypt at restRecovery codes are BCrypt-hashed before storage and verified using Summary
The CRITICAL findings mean this PR should not be merged as-is. The MFA feature gives users a false sense of security: secrets are stored in plaintext, and the MFA policy for sensitive actions is entirely unconnected. Both issues are fixable with targeted changes. |
The entity comment claimed TOTP secrets are encrypted at rest by the infrastructure layer, but no encryption is actually applied. Update the comment to accurately reflect the current plaintext storage and document the need for a future encryption enhancement.
IsMfaRequiredForSensitiveActionAsync was dead code -- never called from any controller. Now the change-password endpoint checks MFA policy and requires a valid TOTP/recovery code when RequireMfaForSensitiveActions is true and the user has MFA enabled. Adds MfaService dependency to AuthController and optional MfaCode field to ChangePasswordRequest.
DisableAsync only validated TOTP codes, meaning users who lost their authenticator app could not disable MFA even with valid recovery codes. Now falls back to TryUseRecoveryCode when TOTP validation fails, matching the pattern already used in VerifyCodeAsync.
OIDC callbacks now include oauth_provider=oidc in the redirect URL. The frontend reads this parameter and routes to the correct exchange endpoint (exchangeOidcCode vs exchangeOAuthCode), fixing incorrect toast messages and error strings for OIDC logins.
AuthController now requires MfaService in its constructor. Add CreateMockMfaService helper and update all test call sites.
Security Review Fixes AppliedFollowing the adversarial review, the following CRITICAL and HIGH findings have been fixed and pushed to this branch: Fixed: C1 -- Misleading encryption-at-rest documentationCommit: Fixed: C2 -- MFA enforcement wired into ChangePassword endpointCommit:
Fixed: H1 -- Recovery code support in DisableAsyncCommit: Fixed: H2 -- Frontend OIDC code exchange routingCommit: Fixed: Test updatesCommit: Remaining findings (MEDIUM/LOW, not fixed in this push)
All 5 commits build cleanly ( |
…e (PRs #806-#813) STATUS.md: add Feature, Security, and Ops Expansion Wave section with full delivery notes for 8 PRs; update backend controller list, auth posture (OIDC/MFA/PKCE), frontend views (calendar/agents/storybook/ import/telemetry); update Phase 4 remaining items. IMPLEMENTATION_MASTERPLAN.md: add delivery history entry #21 with wave summary; update test baseline counts to ~5,600+. TESTING_GUIDE.md: update verified totals to backend ~3,600+, frontend ~1,984+, combined ~5,600+; add Feature/Security Expansion Wave Testing section documenting ~231+ new tests across 8 PRs. decisions/INDEX.md: add ADR-0028 (staged deployment) and ADR-0029 (OIDC/MFA pluggable identity).
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9112435e4b
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| authBuilder.AddOpenIdConnect(schemeName, provider.DisplayName, options => | ||
| { | ||
| options.Authority = provider.Authority; | ||
| options.ClientId = provider.ClientId; | ||
| options.ClientSecret = provider.ClientSecret; |
There was a problem hiding this comment.
This OIDC registration never sets a SignInScheme or adds a cookie handler, while the app default scheme is JWT bearer. In the /oauth-redirect callback flow, the OpenID Connect handler needs a sign-in handler to persist the external principal before redirecting to OidcCallback; with bearer-only auth that callback path fails, so configured OIDC providers can't complete login. Register a cookie scheme for external auth and point OIDC SignInScheme to it.
Useful? React with 👍 / 👎.
| var replayKey = $"{userId.Value}:{code}:{step}"; | ||
| if (_usedCodes.ContainsKey(replayKey)) | ||
| return false; | ||
|
|
||
| // Mark code as used with expiry = 2 * tolerance * timeStep |
There was a problem hiding this comment.
Replay protection is implemented as a ContainsKey check followed by a later write, which is not atomic. Two concurrent requests using the same user/code/time-step can both observe the key as absent and both succeed, allowing one TOTP code to be replayed under parallel load. Use an atomic insert (for example, TryAdd) for the replay key check-and-set.
Useful? React with 👍 / 👎.
Update MANUAL_TEST_CHECKLIST last-updated date and add a new "Z. Outstanding PR Test Backlog" section (2026-04-10). The added backlog aggregates manual test tasks pulled from PR test plans (open PRs #797–#813 and closed PRs #768–#799), includes a status legend, and enumerates verification checklists across categories (Z1–Z17) such as OIDC/SSO & MFA, OAuth PKCE, calendar/timeline views, note import/web clip intake, Storybook, deployment/ops, testing infrastructure (Testcontainers, cross-browser E2E, visual regression, mutation testing), PWA/offline, SignalR scale-out, Postgres migration compatibility, forecasting, inbox UX, CSV export, accessibility, E2E edge scenarios, dependency hygiene, and concurrency stress tests. This provides a single reference for outstanding manual verification items tied to the referenced PRs.
# Conflicts: # backend/src/Taskdeck.Api/Taskdeck.Api.csproj # docs/decisions/INDEX.md
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e50b0825ad
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| var mfaResult = await _mfaService.VerifyCodeAsync(callerUserId, request.MfaCode); | ||
| if (!mfaResult.IsSuccess) | ||
| return StatusCode(StatusCodes.Status403Forbidden, new ApiErrorResponse( | ||
| ErrorCodes.AuthenticationFailed, "Invalid MFA verification code")); |
There was a problem hiding this comment.
When MFA is required, this block verifies the MFA code before attempting the password change. VerifyCodeAsync consumes and persists recovery-code usage, so if the current password is wrong (or new password validation fails), the request is rejected but a one-time recovery code has already been burned. In practice, users can lose backup codes through failed password-change attempts; destructive MFA consumption should happen only after password validation succeeds (or within one transaction that rolls back on failure).
Useful? React with 👍 / 👎.
| <div v-if="status && !status.isSetupAvailable" class="td-mfa-setup__info"> | ||
| MFA setup is not available on this instance. Contact your administrator. |
There was a problem hiding this comment.
This condition hides the entire enabled-state UI whenever isSetupAvailable is false. GetStatus can return isEnabled=true and isSetupAvailable=false (for example, after an admin turns setup off), and in that state users only see the “not available” message and cannot reach the disable flow. The branch should account for enrolled users (e.g., only show this message when setup is unavailable and MFA is not already enabled).
Useful? React with 👍 / 👎.
- MfaService: Store null instead of "EXHAUSTED" sentinel when recovery codes are exhausted - MfaCredential.SetRecoveryCodes: Allow null/empty to clear codes (was throwing on empty) - MfaCredential.Revoke(): Fix doc comment - clarify secret is preserved for audit trail - AuthenticationRegistration: Add SignInScheme cookie for OAuth/OIDC external auth flow - LoginView: Move type import before executable statements - MfaSetup: Fix misleading "scan QR code" copy when no QR is rendered
- MfaService: Store null instead of "EXHAUSTED" sentinel when recovery codes are exhausted - MfaCredential.SetRecoveryCodes: Allow null/empty to clear codes (was throwing on empty) - MfaCredential.Revoke(): Fix doc comment - clarify secret is preserved for audit trail - AuthenticationRegistration: Add SignInScheme cookie for OAuth/OIDC external auth flow - LoginView: Move type import before executable statements - MfaSetup: Fix misleading "scan QR code" copy when no QR is rendered
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 94cc46a04c
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| inputmode="numeric" | ||
| pattern="[0-9]*" | ||
| maxlength="6" | ||
| placeholder="000000" |
There was a problem hiding this comment.
Recovery codes are generated in XXXX-XXXX format, but this input is constrained to a 6-character numeric OTP, so backup codes cannot be entered here. In the common failure scenario where a user loses their authenticator app, they cannot use recovery codes to disable MFA from the UI, defeating the recovery path.
Useful? React with 👍 / 👎.
| if (string.IsNullOrWhiteSpace(username)) | ||
| username = $"{provider.Name.ToLowerInvariant()}-user-{providerUserId}"; |
There was a problem hiding this comment.
This fallback uses the raw OIDC subject (providerUserId) to build a local username without any length guard. User enforces a 50-character maximum username, and ExternalLoginAsync passes this value through to user creation, so providers that emit long subject identifiers will fail login with validation errors instead of creating/signing in the user.
Useful? React with 👍 / 👎.
Backend: The MfaCredentialTests.SetRecoveryCodes_ShouldThrow_WhenEmpty test expected a DomainException to be thrown when calling SetRecoveryCodes with an empty string, but the implementation deliberately allows empty strings to clear recovery codes (e.g., when exhausted). Updated test to verify the clearing behavior. Frontend: The MfaSetup.spec.ts test expected the component to display "Add this shared secret" but the component renders "Add this secret". Updated test to match the actual component text.
# Conflicts: # backend/src/Taskdeck.Api/Controllers/AuthController.cs # backend/src/Taskdeck.Infrastructure/Repositories/UnitOfWork.cs # backend/tests/Taskdeck.Api.Tests/AuthControllerEdgeCaseTests.cs # frontend/taskdeck-web/src/api/authApi.ts
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
Update SetRecoveryCodes test to match domain method: empty/null now clears codes instead of throwing (supports exhausted recovery codes). Remove AuthenticationRegistrationTests (belongs to PR #813, not cache PR). Take main's versions for non-cache files.
Merge both OIDC settings and telemetry settings parameters in SettingsRegistration and Program.cs. Both feature sets coexist.
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
Take main's versions for all non-cache files now that #813 provides OIDC/MFA infrastructure. Keep MfaCredentialTests fix with both empty-string and null clearing test cases.
Summary
Closes #82
Key Design Decisions
MfaPolicy:RequireMfaForSensitiveActions)oidc_{ProviderName}prefix isolates identity namespaces across providersConfiguration
{ "Oidc": { "Providers": [ { "Name": "entra", "DisplayName": "Microsoft Entra ID", "Authority": "https://login.microsoftonline.com/{tenant-id}/v2.0", "ClientId": "your-client-id", "ClientSecret": "your-client-secret", "Scopes": ["openid", "profile", "email"] } ] }, "MfaPolicy": { "EnableMfaSetup": true, "RequireMfaForSensitiveActions": true } }Files Changed
Domain
MfaCredential.cs-- new entity for TOTP credentialsUser.cs-- addedMfaEnabledflag andEnableMfa/DisableMfamethodsApplication
OidcProviderSettings.cs-- pluggable OIDC provider configurationMfaPolicySettings.cs-- MFA policy configurationMfaService.cs-- TOTP generation, validation, setup, and disable flowsMfaDtos.cs/OidcDtos.cs-- request/response DTOsIMfaCredentialRepository.cs-- repository interfaceIUnitOfWork.cs-- addedMfaCredentialspropertyInfrastructure
MfaCredentialConfiguration.cs-- EF Core configurationMfaCredentialRepository.cs-- repository implementation20260409120000_AddMfaCredentials-- adds MfaCredentials table and User.MfaEnabled columnUnitOfWork.cs/DependencyInjection.cs-- DI registrationAPI
MfaController.cs-- MFA endpoints (status, setup, confirm, verify, disable)AuthController.cs-- OIDC login/callback/exchange endpoints, updated providers endpointAuthenticationRegistration.cs-- OpenIdConnect provider registrationSettingsRegistration.cs/Program.cs-- OIDC/MFA settings bindingFrontend
auth.ts-- OIDC and MFA typesauthApi.ts-- OIDC exchange and MFA API client methodsLoginView.vue-- OIDC login buttons (config-gated)sessionStore.ts-- OIDC code exchangeMfaSetup.vue-- MFA setup component for settingsMfaChallengeModal.vue-- MFA challenge modal for protected actionsDocs
ADR-0028-sso-oidc-mfa-integration.md-- design decision recordTest plan
dotnet build backend/Taskdeck.sln -c Release)dotnet test backend/Taskdeck.sln -c Release -m:1)npm run typecheck)npx vitest --run)